Pushdown sort by complex expressions to scan #4750

songkant-aws · 2025-11-06T06:26:14Z

Description

Add sort performance enhancement of pushing down sort by complex expressions to scan.

Related Issues

Resolves #3912

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed.
API changes companion pull request created.
Commits are signed per the DCO using --signoff or -s.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Songkan Tang <[email protected]>

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java

Signed-off-by: Songkan Tang <[email protected]>

core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteSortCommandIT.java

LantaoJin · 2025-11-10T08:44:09Z

.../main/java/org/opensearch/sql/opensearch/planner/rules/ExpandCollationOnProjectExprRule.java

-        project.getInput().getTraitSet().getTrait(RelCollationTraitDef.INSTANCE);
+
+    // Branch 1: Check if complex expressions are already sorted by scan
+    if (handleComplexExpressionsSortedByScan(call, project, toTraits, toCollation)) {


can you add some IT and ExplainIT for this case?

All of sort expression pushdown ITs are for this case.

nit: which one for example?
and can this predicate move to definition of ExpandCollationOnProjectExprRule.Config (can reduce node copy)

...search/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java

...rch/src/main/java/org/opensearch/sql/opensearch/storage/scan/context/SortExpressionInfo.java

opensearch/src/main/java/org/opensearch/sql/opensearch/util/OpenSearchRelOptUtil.java

Signed-off-by: Songkan Tang <[email protected]>

qianheng-aws · 2025-11-13T07:38:31Z

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java

+                                            b2.operand(CalciteLogicalIndexScan.class)
+                                                .predicate(
+                                                    Predicate.not(
+                                                            CalciteLogicalIndexScan


Seems duplicated with the below predication of noAggregatePushed.

Use noAggregatePushed now

qianheng-aws · 2025-11-13T08:16:41Z

opensearch/src/main/java/org/opensearch/sql/opensearch/util/OpenSearchRelOptUtil.java

+
+    // Check if the scan has sort expressions pushed down
+    if (scan.getPushDownContext().stream()
+        .noneMatch(operation -> operation.type() == PushDownType.SORT_EXPR)) {


Is it possible that the pushed SORT(i.e. SORT_EXPR with all digests are simple expression) can satisfy sort collation here?

It's possible. The cost computation has coarse results by multiplying field count with 1.1. So VolcanoPlanner will prefer regular field sort pushdown.

Another option is to calculate the accurate complex expression count and multiply this count with 1.1. Put a check in the rule to not pushdown if all of expressions are simple expressions

I mean, in this query ... | sort a | eval b = a + 1 | sort b, when pushing the sort b, this method will always return false. But I expect it return true.

Yes, it returns false in this case but doesn't matter. Now, if all of expressions are simple, it will go through regular pushDownSort. Field sort exists in regular traitset and is propagated well. It will be handled by handleSimpleExpressionFieldSorts method in ExpandCollationOnProjectExprRule.

qianheng-aws · 2025-11-13T08:18:58Z

opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/SortExprIndexScanRule.java

+      Integer offsetValue = LimitIndexScanRule.extractOffsetValue(sort.offset);
+      newScan = (CalciteLogicalIndexScan) scan.pushDownLimit(sort, limitValue, offsetValue);
+    } else {
+      // Attempt to push down sort expressions


We still push down sort although scanProvidesRequiredCollation is true? I think we could just remove the sort directly.

I was thinking if first sort by [a, b] expressions is already in scan. The second sort by [a] expression could narrow down the sort effort. We could still allow the pushdown.

For topK pushdown, it could be also applied. If sort by [a, b], limit 10 is pushed, say the second sort by [a], limit 2 comes in, we should allow it pushdown as well. This may need replace existing SORT_EXPR and LIMIT pushdown context.

But right now, it has the problem that multiple different sort complex expressions doesn't work well yet.

The second sort by [a] expression could narrow down the sort effort. We could still allow the pushdown.

Shall we do the same thing for the above branch then? It only push down limit when scanProvidesRequiredCollation is true.

Revisit the logic, for already pushed down topK and if the scan collation satisfies the output collation, we can still leave the old sort in the scan(remove the new sort in the meantime). It's kind of keeping semantics. For regular sort pushdown, we can override by the new sort.

qianheng-aws · 2025-11-13T08:22:36Z

opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java

        throw new UnsupportedScriptException(
            "ScriptQueryExpression requires a valid current time from hook, but it is not set");
      }
+      Map<String, Object> mergedParams = new HashMap<>(params);


Better use LinkedHashMap here or the final plan may be unstable.

Use LinkedHashMap now

qianheng-aws · 2025-11-13T08:25:55Z

...search/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalIndexScan.java

+      OSRequestBuilderAction action =
+          requestBuilder -> {
+            for (SortExprDigest info : sortExprDigests) {
+              requestBuilder.pushDownSortExpression(


[question] Will this method throw exception by any chance? We plan to make the request builder transformation process lazy. We should put any logic which may throw exception outside of the lambda function, so it can avoid failure on the final plan.

As discussed offline, move most of logic that could throw exception outside. And use a lazy pushdownSortSuppliers method instead.

qianheng-aws · 2025-11-13T08:29:43Z

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java

+                sortExprDigest.getDirection().name()));
+
+    Script script = scriptExpr.getScript();
+    if (script != null) {


What if script is null? Seems nothing happens. We better add assertion or throw exception if that's never possible.

It should never happen. I neglect this check.

qianheng-aws · 2025-11-13T08:48:43Z

...earch/src/main/java/org/opensearch/sql/opensearch/storage/scan/AbstractCalciteIndexScan.java

+        case SORT_EXPR -> {
+          @SuppressWarnings("unchecked")
+          List<SortExprDigest> sortKeys = (List<SortExprDigest>) operation.digest();
+          dCpu += NumberUtil.multiply(dRows, 1.1 * sortKeys.size());


I think we should filter the simple expr.

Now, it will only count complex expr. The calculation is fine grained.

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws added 4 commits November 5, 2025 17:49

Pushdown sort expression to scan

c85bb1a

Signed-off-by: Songkan Tang <[email protected]>

Merge branch 'main' into sort-expression-pushdown

cadac00

Refactor a bit of SortExprIndexScanRule

cc61524

Signed-off-by: Songkan Tang <[email protected]>

Simplify some code and add more explain tests

3bc9d62

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws marked this pull request as ready for review November 6, 2025 09:38

songkant-aws commented Nov 6, 2025

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java Outdated Show resolved Hide resolved

songkant-aws commented Nov 6, 2025

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchRequestBuilder.java Outdated Show resolved Hide resolved

yuancu added enhancement New feature or request backport 2.19-dev labels Nov 7, 2025

songkant-aws added 3 commits November 7, 2025 17:46

Support null ordering for sort expression pushdown

d1bd892

Signed-off-by: Songkan Tang <[email protected]>

Attempt to fix security tests and add more test cases

d21ecef

Signed-off-by: Songkan Tang <[email protected]>

Merge branch 'main' into sort-expression-pushdown

0492cdf

LantaoJin reviewed Nov 10, 2025

View reviewed changes

Address comments

99580ff

Signed-off-by: Songkan Tang <[email protected]>

LantaoJin previously approved these changes Nov 12, 2025

View reviewed changes

qianheng-aws reviewed Nov 13, 2025

View reviewed changes

Address more comments

9714ed8

Signed-off-by: Songkan Tang <[email protected]>

songkant-aws dismissed LantaoJin’s stale review via 9714ed8 November 14, 2025 09:40

Pushdown sort by complex expressions to scan #4750

Are you sure you want to change the base?

Pushdown sort by complex expressions to scan #4750

Uh oh!

Conversation

songkant-aws commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songkant-aws Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qianheng-aws Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songkant-aws Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

songkant-aws commented Nov 6, 2025 •

edited

Loading

LantaoJin Nov 12, 2025 •

edited

Loading

songkant-aws Nov 14, 2025 •

edited

Loading

qianheng-aws Nov 13, 2025 •

edited

Loading

songkant-aws Nov 14, 2025 •

edited

Loading